Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dotnet: infrastructure improvements #336824

Merged
merged 36 commits into from
Sep 17, 2024
Merged

Conversation

corngood
Copy link
Contributor

@corngood corngood commented Aug 23, 2024

Description of changes

The most important parts of this change are:

  • dotnet-sdk-setup-hook: configure nuget in sourceRoot

    • Previously we created a nuget.config in $NIX_BUILD_TOP (or wherever genericBuild was executed from. Instead, we now attempt to make the changes to the root nuget.config in sourceRoot, creating a default one if it doesn't exist.
    • This should fix things like clps2c-compiler: init at 1.0.1 #339716.
    • It uses quite a bit of hairy xmlstarlet logic that I would like to eventually move to something more robust (C# maybe, if bootstrapping can be done in a sane way?).
  • dotnetCorePackages: split fetch-deps logic into addNuGetDeps

    • This allows fetch-deps to be added to any stdenv package, not just via buildDotnetModule.
    • msbuild: use addNuGetDeps is a good example of how this is used
  • avalonia: init at 11.0.10

    • Add source-built avalonia package.
    • This is currently only used in nexusmods-app. I've tested all other Avalonia 11 apps with it, but I want to get a better idea of how upgrading is going to work before forcing it on all maintainers.
    • avalonia: 11.0.10 -> 11.0.11
      • This demonstrates how an upgrade would work.
  • add mapNuGetDependencies

    • This is an opt-in setting that adds all packages found in nix dependencies to packageSourceMapping.
    • It's currently only used in nexusmods-app in order to force it to use avalonia 11.0.11.
    • I see this potentially being the default at some point.

Supporting work:

Unrelated fixes to problems I found along the way:

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Contributor

@GGG-KILLER GGG-KILLER left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few initial comments

pkgs/build-support/dotnet/build-dotnet-module/default.nix Outdated Show resolved Hide resolved
@@ -25,7 +25,6 @@ runCommandLocal "nuget-to-nix" {
curl
gnugrep
gawk
dotnet-sdk
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be a breaking change since it now retrieves the dotnet from path (which isn't guaranteed to be there)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm still not sure this is a good idea, but having to override it all over the place seems wrong.

with dotnetCorePackages;
combinePackages [
sdk_6_0
sdk_7_0_1xx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really add a new dependency on .NEY 7 when it's EOL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I specifically chose the version of avalonia used by nexusmods-app, and it's pinned to 7.0.1xx. Newer versions are on 8 I believe.

I'll do some experimenting with newer versions, but I don't think it's a worse situation than pulling the binary blob, which was also built with the EOL sdk.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a situation where it'd be useful to have multiple versions of avalonia packaged? Or perhaps that's jumping the gun, if it's currently only used by nexusmods-app...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that we won't have to do any more than package multiple major versions of packages.

For example, right now I see things in nixpkgs using avalonia:

  • 0.10.12
  • 0.10.13
  • 0.10.15
  • 0.10.18
  • 11.0.4
  • 11.0.5
  • 11.0.9
  • 11.0.10
  • 11.0.11

So maybe we'd get away with just avalonia_11, until 12 exists? 0.10 could probably stay as binary. Some work will probably still have to be done to relax version constraints and ensure we're prioritising packages from nixpkgs.

Another good example is newtonsoft.json. To support all major versions currently used in nixpkgs, we'd need newtonsoft-json_9, _10, _11, _12, and _13.

Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this goes a little over my head, but I like the high level direction!

Speaking from the nexusmods perspective, this seems useful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to see 14 deps dropped here.

Slightly confused why there's still a handful of Avalonia deps in the file.

Also confused why running fetch-deps has given us new hashes for each dep even though their versions are unchanged?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly confused why there's still a handful of Avalonia deps in the file.

Some are from separate repos I believe (buildsystem?), but some are just older versions, which is something I need to investigate.

Also confused why running fetch-deps has given us new hashes for each dep even though their versions are unchanged?

I believe that's just changing to SRI hashes, and the actual hash values aren't actually changed.

pkgs/by-name/av/avalonia/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ne/nexusmods-app/package.nix Show resolved Hide resolved
pkgs/by-name/ne/nexusmods-app/package.nix Show resolved Hide resolved
@corngood corngood force-pushed the dotnet-avalonia branch 2 times, most recently from 48b508d to 574ebcb Compare August 23, 2024 19:52
@ofborg ofborg bot requested a review from MattSturgeon August 23, 2024 21:01
@js6pak
Copy link
Contributor

js6pak commented Sep 5, 2024

add avalonia as a source-built package and patch it so it can find libX11 etc from nixpkgs

It would also be fairly easy to make a patchelf-like tool that replaces the shared library names in DllImportAttributes and/or inject an DllImportResolver inside built IL assembly files.
I could make one if you think it's a good way to go about getting rid of runtimeDeps.

@corngood
Copy link
Contributor Author

corngood commented Sep 6, 2024

I've split some of this stuff into: #339953.

@js6pak patching sounds like an interesting idea. The PR I referenced has the package overriding mechanism in it, and you can see how it adds the runtime deps specifically for Avalonia.X11. If that was replaced with assembly patching, it would be more robust.

@corngood
Copy link
Contributor Author

corngood commented Sep 8, 2024

I've been doing some more work on the source-built avalonia, and I'm trying to figure out the best way to consume it in other packages.

Say we add the package avalonia, and try to keep it up to date. That means we'd have avalonia 11.1.3 in nixpkgs. We can add it as a dependency, and if the dependent package is looking for that exact version, it will use it. If it's looking for e.g. 11.0.10, it won't be used. This is because of:

https://learn.microsoft.com/en-us/nuget/concepts/dependency-resolution#lowest-applicable-version

I can think of a couple of ways of approaching this.

  1. use packageSourceMapping to map these packages to the _nix source, which will make them the only available versions in the build.

  2. patch the dependent projects to use the correct version, or possibly floating versions?

I've implemented (1), and it's promising, but you can't map by version (NuGet/Home#13351), so you can't really have a mix of versions from nuget.org and nixpkgs for a specific package. It also complicates things because turning on packageSourceMapping requires all sources in the project to use it (so we have to do a bunch more nuget.config patching).

(2) is more work per-package, and if you use floating versions, your source-built dependency can be overridden in fetch-deps when a newer version is released on nuget.org.

Anyone have any thoughts/ideas? @GGG-KILLER @js6pak @MattSturgeon maybe?

@MattSturgeon
Copy link
Contributor

MattSturgeon commented Sep 8, 2024

My first thought is that if we want fuzzy versions, this should be up to the package to declare. Replacing nuget x.y.z with source-built x.y.z should usually be fine, but doing fuzzy matching like x.y.* could be more likely to introduce unexpected issues?

Alternatively, instead of doing version matching, we could simply have a insteadOfNuget inputs set? An attrset mapping nuget names to derivations to be used instead of what is listed in deps.nix.

This has the pro and con of being more explicit about which inputs are handled by nuget and which come from nixpkgs.

@corngood
Copy link
Contributor Author

corngood commented Sep 8, 2024

My first thought is that if we want fuzzy versions, this should be up to the package to declare. Replacing nuget x.y.z with source-built x.y.z should usually be fine, but doing fuzzy matching like x.y.* could be more likely to introduce unexpected issues?

Well a dependency like this from nexusmods-app:

<PackageVersion Include="Avalonia" Version="11.0.10" />

Really means >= 11.0.10, so if only 11.0.11 (or 12.0.0 for that matter) exists, it will use it without complaining.

NuGet calls normal semver dependencies 'floating versions'. If those were more widely used (for example nexusmods-app could probably use 11.0.* or 11.*), then this would be more intuitive IMO.

@GGG-KILLER
Copy link
Contributor

GGG-KILLER commented Sep 9, 2024

Well a dependency like this from nexusmods-app:

<PackageVersion Include="Avalonia" Version="11.0.10" />

Really means >= 11.0.10, so if only 11.0.11 (or 12.0.0 for that matter) exists, it will use it without complaining.

Given this, couldn't we have the option of just packaging the latest version for each major without needing to change anything?

@corngood
Copy link
Contributor Author

corngood commented Sep 9, 2024

Given this, couldn't we have the option of just packaging the latest version for each major without needing to change anything?

Yeah, sort of, but only if you use the package source mapping to force it to pick packages from nixpkgs instead of nuget.org.

If nixpkgs has 11.0.11, then nugetmods-app will use it if it's the only option. If it can find both 11.0.11 (from nixpkgs) and 11.0.10 (from nuget.org), it'll use the later due to 'lowest applicable version'.

I personally think we should force the nixpkgs dependencies, as long as they meet the project constraints. The default behaviour (i.e. 'lowest applicable version') seems like a nightmare for rolling out security fixes. We'd also be encouraging projects to declare dependencies more carefully than just >= 11.0.10.

@GGG-KILLER
Copy link
Contributor

Yeah, sort of, but only if you use the package source mapping to force it to pick packages from nixpkgs instead of nuget.org.

If nixpkgs has 11.0.11, then nugetmods-app will use it if it's the only option. If it can find both 11.0.11 (from nixpkgs) and 11.0.10 (from nuget.org), it'll use the later due to 'lowest applicable version'.

But given that we use a directory as a source on the dotnet restore stage, how would it get in touch with nuget.org? Or do you mean on the fetch-deps stage?

Instead of creating a nuget.config in NIX_BUILD_TOP, and depending on
inheritance from sourceRoot, we'll make the required changes directly in
the latter.  This fixes packages where sourceRoot is NIX_BUILD_TOP.
We now handle creating TMPDIR and setting the working directory outide
of nix-shell.  This allows nix-shell to set NIX_BUILD_TOP correctly.
xq has such a long startup time that this significantly improves
performance.
linkNugetPackages and linkNuGetPackagesAndSources can now be disabled by
setting them to false.

linkNugetPackages will use _linkPackages, which is much faster.
This allows the script to run on platforms that can't fetch all of the
packages, or without allowing unfree.
This allows you to resume after a failure by passing the name of the
first package you want to be fetched.
@corngood corngood marked this pull request as ready for review September 17, 2024 04:15
@corngood
Copy link
Contributor Author

But given that we use a directory as a source on the dotnet restore stage, how would it get in touch with nuget.org? Or do you mean on the fetch-deps stage?

Sorry @GGG-KILLER, I missed this question.

Yeah, I was thinking of fetch-deps. If it's able to find a "better" (older, and closer to the minimum version) package on nuget.org it'll download it and add it to the lockfile.

@corngood corngood merged commit 1b7f8c9 into NixOS:master Sep 17, 2024
25 of 27 checks passed
@huantianad
Copy link
Contributor

Hi, sorry just saw this and the changes for Scarab, should I be making a PR upstream to include this change?

@corngood corngood deleted the dotnet-avalonia branch September 17, 2024 17:17
@corngood
Copy link
Contributor Author

@huantianad possibly. I found a few others that did the same thing. It's not really wrong exactly, it's just depending on the user's global nuget configuration. It's pretty easy to hack around in nixpkgs, and might even be worthy of a shared hook/option.

A lot of projects are explicit about package sources like this: https://github.com/AvaloniaUI/Avalonia/blob/master/NuGet.Config. Other projects don't even specify a nuget configuration. In those cases we create a default config which includes nuget.org.

Maybe we should be adding nuget.org when there isn't a <clear/> in the root config?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants